Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: 🗑️ Deprecation notice for js-ipfs #1628

Merged
merged 3 commits into from
Jun 30, 2023
Merged

fix: 🗑️ Deprecation notice for js-ipfs #1628

merged 3 commits into from
Jun 30, 2023

Conversation

whizzzkid
Copy link
Contributor

@whizzzkid whizzzkid commented Jun 17, 2023 •
edited
Loading

Describe your changes

Files changed

  • docs/how-to/companion-node-types.md

What issue(s) does this address?

Checklist before requesting a review

  • Passing the beta version of the Check Markdown links for modified files check. Action results can be viewed here.

Checklist before merging

  • Passing all required checks (The beta Check Markdown links for modified files check is not required)

@whizzzkid whizzzkid requested a review from SgtPooki June 17, 2023 10:24
@welcome
Copy link

welcome bot commented Jun 17, 2023

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

  • The context for this PR is clear, with relevant discussion, decisions and stakeholders linked/mentioned.
  • Your contribution itself is clear (grammar and spelling checked, any code blocks checked and commented) and in its best form. Follow the docs contribution guidelines if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

  • A maintainer will triage and assign priority to this PR, commenting on any missing things and potentially assigning a reviewer for high priority items.
  • The PR gets reviews, discussed and approvals as needed.
  • The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

@whizzzkid whizzzkid requested a review from lidel June 17, 2023 10:26
@BigLep
Copy link
Contributor

BigLep commented Jun 19, 2023

Per ipfs/ipfs-companion#1220 (comment), I would encourage removing this all-together. I assume we aren't testing this functionality and it's not supported. As a result, I don't see value in advertising it.

@ElPaisano ElPaisano self-requested a review June 20, 2023 17:37
SgtPooki
SgtPooki approved these changes Jun 26, 2023
View reviewed changes
Copy link
Member

@SgtPooki SgtPooki left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would add a tracking issue on the companion side for users to follow so they will know when it's done.

Closing that issue would also be a good indicator that these docs need updating again.

Also, +1 to what Steve said.

@whizzzkid
Copy link
Contributor Author

@SgtPooki the checks seem to be failing because of an error link in a file that didn't change. merge with errors? 🤔

ElPaisano
ElPaisano approved these changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@ElPaisano ElPaisano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a slight tweak

docs/how-to/companion-node-types.md Outdated Show resolved Hide resolved
@ElPaisano
Copy link
Contributor

@whizzzkid are there any other changes you'd like to make before I merge this into https://github.com/ipfs/ipfs-docs/tree/js-ipfs-cleanup for final cleanup and merging?

@ElPaisano ElPaisano merged commit e924363 into js-ipfs-cleanup Jun 30, 2023
@ElPaisano ElPaisano deleted the fix/companion-node-type-docs branch June 30, 2023 01:43
@BigLep BigLep mentioned this pull request Jun 30, 2023
Merged
1 task
BigLep
BigLep reviewed Jun 30, 2023
View reviewed changes
Copy link
Contributor

@BigLep BigLep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put a couple of comments but ultimately created a new PR here #1631 @whizzzkid

docs/how-to/companion-node-types.md
Comment on lines +70 to +74
::: warning DEPRECATED

js-ipfs has been deprecated in favor of [Helia](https://github.com/ipfs/helia) and will be removed in a future release of IPFS Companion. Improvements to Helia are ongoing, and we hope to have it ready for use in IPFS Companion once critical features like [webRTC Streams](https://github.com/w3c/webextensions/issues/72) are available in Chrome Manifest V3.

:::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whizzzkid : I think this just adds noise. This can be in our backlogged but this was a non-functioning feature. I think we should just strip it out.

docs/how-to/companion-node-types.md
@@ -67,6 +67,12 @@ This node type offers the same benefits as an [external](#external) node, with a

## Embedded
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just remove the full Embedded section?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BigLep BigLep BigLep left review comments

@ElPaisano ElPaisano ElPaisano approved these changes

@SgtPooki SgtPooki SgtPooki approved these changes

@lidel lidel Awaiting requested review from lidel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@whizzzkid @BigLep @ElPaisano @SgtPooki